Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PHP 8.4 support added #253

Closed
wants to merge 4 commits into from

Conversation

Atul-glo35265
Copy link

Q A
Documentation yes/no
Bugfix yes/no
BC Break yes/no
New Feature yes/no
RFC yes/no
QA yes/no

Description

Copy link
Member

@gsteel gsteel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation linting problems will need to be fixed prior to merge here - this is due to CI improvements and unrelated to your patch.

Please also sign off your commits as per https://github.com/laminas/.github/blob/main/CONTRIBUTING.md#commit-signoffs

Signed-off-by: Atul-glo35265 <glo35265@adobe.com>
Signed-off-by: Atul-glo35265 <glo35265@adobe.com>
@gsteel
Copy link
Member

gsteel commented Oct 9, 2024

All implicit nulls need dealing with here. There's also a blocker in laminas/laminas-stdlib that will need dealing with.

Signed-off-by: Atul-glo35265 <glo35265@adobe.com>
@Atul-glo35265
Copy link
Author

Atul-glo35265 commented Oct 14, 2024

All implicit nulls need dealing with here. There's also a blocker in laminas/laminas-stdlib that will need dealing with.

Hi @gsteel, To fix the laminas/laminas-mvc related issues here, do we need to raise another PR with the fix, for laminas-mvc:3.7.0 as 3.7.0 is being used in the current repo?
Please advise.

Copy link
Member

@gsteel gsteel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you bump a dependency, you must also update the lock file otherwise CI will fail across the board.

@Atul-glo35265 Atul-glo35265 force-pushed the php8.4_support branch 2 times, most recently from 6189610 to fa34b3a Compare October 14, 2024 09:16
@Atul-glo35265
Copy link
Author

When you bump a dependency, you must also update the lock file otherwise CI will fail across the board.

I think bumping dependency will not work here, due to other dependencies. Please check this comment and advise.

@gsteel
Copy link
Member

gsteel commented Oct 14, 2024

Looks like a release of MVC 3.8.0 is required before upgrading to 8.4 here is possible

@Atul-glo35265
Copy link
Author

Atul-glo35265 commented Oct 15, 2024

Looks like a release of MVC 3.8.0 is required before upgrading to 8.4 here is possible

@gsteel, can you please confirm, when laminas/laminas-mvc:3.8.0 release is planned and will 3.8.0 come with php8.4 support? Actually, we are working on Magento Dependencies and php8.4 support. We will be blocked, due to laminas dependencies.

@gsteel
Copy link
Member

gsteel commented Oct 17, 2024

You need to bump the minimum version of PHPUnit 10 to the most recent version in the 10.x series.

I will also need to fix docs linting issues in a different patch.

Signed-off-by: Atul-glo35265 <glo35265@adobe.com>
@Atul-glo35265
Copy link
Author

You need to bump the minimum version of PHPUnit 10 to the most recent version in the 10.x series.

I will also need to fix docs linting issues in a different patch.

I've updated the phpunit10 version to 10.5.36.

@gsteel
Copy link
Member

gsteel commented Oct 18, 2024

There are still a lot of implicit null params in src and tests here, along with code in dependencies, specifically MVC which you've already fixed in laminas/laminas-mvc#172 - This patch will need to wait for a release of MVC as well as fixing any remaining compatibility issues directly in view src and tests.

@HeenaBansal20
Copy link

@Atul-glo35265 , When these changes will be merged ?

@gsteel
Copy link
Member

gsteel commented Nov 21, 2024

This has been handled in #255 thanks @Atul-glo35265

@gsteel gsteel closed this Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants